Switch to Microsoft.Extensions.Logging.Abstractions #191
Conversation
Don't abuse the transitive dependency on DependencyInjection
|
Thanks for the PR! This seems like it would cause significant pain downstream - many consumers of this package use |
|
Yeah, maybe it should be added back, but by adding a explicit dependency on Microsoft.Extensions.DependencyInjection. Alternatively, that should live in a different assembly, but that is not nice either. EDIT: Actually, The same goes for the ProviderFilter attribute I removed. EDIT 2: I've just pushed a new commit where I added a separate library for the extension. Let me know if this is the route you want to go. |
|
I don't know if it is related but we're about to completely dump Serilog over a MethodNotFound exception that seems to plague projects using the Microsoft Logging Abtractions. PowerShell/PowerShellEditorServices#1477 I happened across this pull request and perhaps it would solve our issues. I haven't 100% nailed it down, but it appears that when a calling process is running .Net 5, AddSerilog() is simply not available. The odd part is when I add the full csproj to my solution and mark it as a project dependency it is magically resolvable. PowerShellEditorServices isn't directly importing this extension, but it calls OmniSharp which appears to be using it. Thoughts? |
|
@dkattan thanks for the ping. I think the tracking issue on the Serilog side is serilog/serilog-sinks-file#135 - will centralize discussion there. |
|
@dkattan you could as an experiment try grabbing the artifacts on the AppVeyor build here: https://ci.appveyor.com/project/serilog/serilog-framework-logging/builds/39654298 and see how that works out for you. |
| @@ -0,0 +1,16 @@ | |||
| <Project> | |||
There was a problem hiding this comment.
project uses two-spaces indentation for other csprojs
| <Project> | ||
| <ItemGroup> | ||
| <PackageVersion Include="Serilog" Version="2.10.0" /> | ||
| <PackageVersion Include="Serilog.Sinks.Console" Version="3.1.1" /> |
There was a problem hiding this comment.
| <PackageVersion Include="Serilog.Sinks.Console" Version="3.1.1" /> | |
| <PackageVersion Include="Serilog.Sinks.Console" Version="4.0.0" /> |
| @@ -0,0 +1,37 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
New project containing only single file?
There was a problem hiding this comment.
@Cheesebaron If you want to get rid of Microsoft.Extensions.Logging/Microsoft.Extensions.DependencyInjection dependencies then look into MS sources :
internal static class ProviderAliasUtilities
{
private const string AliasAttibuteTypeFullName = "Microsoft.Extensions.Logging.ProviderAliasAttribute";
internal static string GetAlias(Type providerType)
{
foreach (CustomAttributeData attributeData in CustomAttributeData.GetCustomAttributes(providerType))
{
if (attributeData.AttributeType.FullName == AliasAttibuteTypeFullName)
{
foreach (CustomAttributeTypedArgument arg in attributeData.ConstructorArguments)
{
Debug.Assert(arg.ArgumentType == typeof(string));
return arg.Value?.ToString();
}
}
}
return null;
}
}😉 I will not tell you exactly why they did that, but since they did it, then the reason was and why not use such a opportunity?
|
@nblumhardt I've started implementing Serilog using the Microsoft.Extensions.Logging package as well. We've got .NET 6 and Framework 4.7.1 projects that we're straddling, but want to introduce the common interfaces from Microsoft. I've had to manually pick Microsoft.Extensions.Logging version 2.0.0 (deprecated) instead of 2.2.0. I can open a new issue/PR if need be, but thought this issue related: can we at the very least version bump to 2.2.0 ? |
|
@stphnlwlsh a separate PR that addresses the dependency version only would be welcome - this PR is unlikely to move forward at this point due to the potential for breaking changes downstream. Thanks for the note! |
@nblumhardt I suggest to close this abandoned PR since it mixes changes in CI with actual code changes. #214 upgrades the code base and CI without any actual code changes. |
|
Of course switching to |
This PR adresses the following: